Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix API endpoint: Return 401 when the token is missing #133

Closed
wants to merge 1 commit into from
Closed

Fix API endpoint: Return 401 when the token is missing #133

wants to merge 1 commit into from
+83 −5

Conversation

josecelano
Copy link
Member

@josecelano josecelano commented Dec 21, 2022 •
edited
Loading

The API returned a 500 code when the token was missing. I've changed it, and now it returns a 401 code. The change affects all endpoints.

I think we could make other changes:

  • Refactor: create a custom Reject for all the errors. Like the one I have added: struct Unauthorized.
  • Refactor: move routes to their own mod routes.rs.
  • The authenticate function should also return a 401 response instead of token not valid when the token is present but not valid.
  • The GET /api/torrent/:info_hash endpoint should return a 404 when the torrent does not exist and the token is valid.
  • The endpoint GET /api/torrent/:info_hash should return a 404 when the torrent does not exist and the token is invalid. See Insecure Direct Object References (IDOR)). We should not expose that the tracker has a given torrent because the tracker could be private.
  • The DELETE /api/whitelist/:info_hash endpoint should return: a "204 No Content" when it deletes the torrent and a "404 Not Found" when the torrent does not exist or the token is invalid.
  • The POST /api/whitelist/:info_hash endpoint should return a "201 Created" response with the location of the new resource GET /api/whitelist/:info_hash. In this case we would need to create that new endpoint. It could be empty. It could be used only to know if a torrent is whitelisted. Alternatively, we could consider the whitelist a unique resource and use a PUT /api/whitelist/:info_hash to add a new torrent to the list.
  • The POST /api/key/:seconds_valid endpoint should return a "201 Created".
  • The DELETE /api/key/:key should should return "204 No Content".
  • The GET /api/whitelist/reload endpoint should be a POST, PUT or PATCH. This endpoint is a little weird, it's like a remote command. I think I should have use POST /api/commands with the command you want to execute in the request body or POST /api/reload-whitelist.
  • Same for GET /api/keys/reload

@WarmBeer @da2ce7 do you think those changes make sense? If you do, should I make them now or postpone them?

@josecelano josecelano linked an issue Dec 21, 2022 that may be closed by this pull request
Wrong HTTP response code from API for unauthorized request #58
Closed
@josecelano
Copy link
Member Author

josecelano commented Dec 21, 2022 •
edited
Loading

The "test" check is failing because there are some clipply warnings/errors that have been fixed in other pending-to-merge PRs. I'm not going to fix them. I will probably merge those PRs tomorrow and rebase this PR before fixing the newly detected ones.

I have also added one test, "should_return_an_internal_error_server_when_the_token_is_invalid" that has to be changed. In the current behaviour, the API returns a 500 when the token is invalid, and I think it should return a 401.

@josecelano
Copy link
Member Author

Today @WarmBeer @da2ce7 and I agreed on the following tasks. Each task could be an independent PR:

Add tests

  • Keep the current API URLs.
  • Write tests for all endpoints and all cases for each endpoint.
  • Refactor to use one struct for each error (rejection).

Reimplement the current API using Axum

  • Reimplement the API using Axum.

New API (same endpoints, different response codes)

We start building a new API using the /v1 prefix. For example: /v1/api/stats. We can use the same URLs but changing the response codes.

  • Add a prefix to the URL with the API version: /v1/api/.
  • Change the response codes to the right one: 200,201,404, etcetera. Right now, it only uses 200 and 500.

NOTES:

  • We can keep the old API in parallel until we release the new major version.

Long-term API

In the future, we want to:

  • Use a header parameter for the API version instead of a URL prefix. Like this.
  • Use a schema (most likely https://json-ld.org/).
  • Re-design the API and consider other endpoints like the ones suggested here by @dev1z

@josecelano josecelano mentioned this pull request Dec 23, 2022
Closed
4 tasks
@josecelano
Copy link
Member Author

This PR has been reorganized. The new issue is #141

@josecelano josecelano closed this Dec 23, 2022
@josecelano josecelano deleted the issue-58-fix-http-code-for-unauthorized-request branch January 16, 2023 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@da2ce7 da2ce7 Awaiting requested review from da2ce7

@mickvandijke mickvandijke Awaiting requested review from mickvandijke

Assignees
No one assigned
Labels
None yet
Projects
Torrust Solution
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong HTTP response code from API for unauthorized request
1 participant
@josecelano